Skip to content

Conversation

@hlfan
Copy link
Collaborator

@hlfan hlfan commented May 10, 2025

Ditching the ever-increasing amount of code required to accommodate CalHeatmap like in #5906 and going for a custom responsive solution.
Fixes #5905 and closes #5906. Also fixes #5810.
This also makes the history links of the heatmap tab-navigatable, even before hovering over each link.

@HolgerJeromin
Copy link
Collaborator

Nice. 🥳
Can you add a screenshot when ready?

@hlfan hlfan force-pushed the grid-heatmap branch 3 times, most recently from 3e35cf0 to 966bd97 Compare May 11, 2025 17:22
@github-actions
Copy link

1 Warning
⚠️ Number of updated lines of code is too large to be in one PR. Perhaps it should be separated into two or more?

Generated by 🚫 Danger

@hlfan hlfan force-pushed the grid-heatmap branch 3 times, most recently from 064cf0e to 699ff14 Compare May 12, 2025 21:31
@hlfan
Copy link
Collaborator Author

hlfan commented May 12, 2025

default (ISO 8601):
localhost_3000_user_hltest

"ar-AF" locale:
localhost_3000_user_hltest (1)

"dv-MV" locale:
localhost_3000_user_hltest (2)

@hlfan hlfan marked this pull request as ready for review May 12, 2025 21:31
@kcne
Copy link
Contributor

kcne commented May 13, 2025

Even though I spent quite a bit of time trying to make our custom configuration work alongside cal-heatmap, this approach makes a bit more sense. Modifying the library ended up requiring more code than writing the config itself.

I really like that this removes the third-party dependency and results in a much cleaner, more maintainable, and self-contained solution.

Just a quick question: what border-radius did you end up using for the squares? It seems to vary slightly across the three screenshots. Also, slightly increasing the spacing between columns and rows could help make the heatmap look a bit more balanced and visually appealing.

Thank you.

@hlfan
Copy link
Collaborator Author

hlfan commented May 13, 2025

It's always 1px from bootstrap, although I certainly don't like to send 3.5 kb of rounded-1 I've tried to use bootstrap for most of the styling.
The seeming variation comes from the grid scaling to the available width (see how the heading apparently scales as well), so the grid gap should maybe be given as a percentage?

@tomhughes
Copy link
Member

With this approach is the javascript actually doing anything that couldn't be done in the rails rendering on the server?

@hlfan
Copy link
Collaborator Author

hlfan commented May 13, 2025

I've stopped just short of the layout because it depends on the CLDR's supplemental weekData which I can use with Intl.locale.

The TwitterCldr gem doesn't have the supplemental data necessary for doing the layout in Ruby.

Nothing is stopping us from reading https://github.com/unicode-org/cldr-json/blob/main/cldr-json/cldr-core/supplemental/weekData.json ourselves, but that needs territories as the input, not just the locale, and idk if that information is available to the server (request with en might mean US or GB which have different weekData).

@hlfan
Copy link
Collaborator Author

hlfan commented May 15, 2025

I've now moved the bootstrap styling to the CSS, saving about a dozen+ kB in the page response size.
Also data-month now counts beyond 12 so I don't have to deal with jQuery selecting both labels of the same month.
And I've fixed the month labels when the calendar reaches into a month by adding more padding for the month label generation.

@kcne
Copy link
Contributor

kcne commented May 15, 2025

I've noticed that the border radius of the day cells changes between desktop and mobile — it looks slightly rounded on desktop but becomes almost pill-shaped on mobile. If this isn’t intentional, we might want to set a fixed border-radius to ensure visual consistency across breakpoints.

Desktop:
Screenshot 2025-05-15 at 10 09 28

Mobile:
Screenshot 2025-05-15 at 10 09 35

On macOS using firefox, when scrolling horizontally, the scrollbar overlaps with the heatmap area. It might be a good idea to add some padding at the bottom of the container to prevent this overlap and improve the scrolling experience.

Screenshot 2025-05-15 at 10 05 17

It might also be a good idea to apply light/dark variants for tooltips based on the current theme to keep visual consistency.

@hlfan
Copy link
Collaborator Author

hlfan commented May 15, 2025

we might want to set a fixed border-radius

This is what is currently being applied with Bootstrap's rounded-1, but I agree a relative border-radius looks more consistent.

the scrollbar overlaps with the heatmap area

I've added mb-1 as a compromise between thick overlaid and classic scrollbars, as I can't get scrollbar-gutter to produce the desired effect.

apply light/dark variants for tooltips

That's what Bootstrap already does, although to prevent the tooltip "merging" with the background and for better accessibility, dark tooltips are used in light mode and light ones in dark mode.

@hlfan hlfan requested a review from tomhughes May 15, 2025 14:59
Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think this is good now.

@tomhughes tomhughes merged commit 32f2346 into openstreetmap:master May 15, 2025
16 checks passed
@verhovsky
Copy link

verhovsky commented May 16, 2025

I don't like the green. Also, it should show the current day, right now on May 16th I only see up to May 15.

@hlfan
Copy link
Collaborator Author

hlfan commented May 16, 2025

Could this be a new edition of #5802 but for days? And caching is probably also involved.

Which green would you prefer (and for which color scheme)?

In general it would be better to open a new issue for that though.

@verhovsky
Copy link

Which green

The same one as github

Screenshot 2025-05-16 at 15 09 05

and I think their implementation of just 5 color buckets is easier to read than a continuous gradient.

Also, it would be cool if there was a "See more" button that would load the entire contribution graph since account creation, like github lets you click through the years

Screenshot 2025-05-16 at 15 11 07

@hlfan
Copy link
Collaborator Author

hlfan commented May 16, 2025

The yearly history has already been mentioned in #5829, which should IMO be a precondition for that.

And anything involving different light and dark color palletes requires watching the color scheme media query again, which I really want to avoid.

@nenad-vujicic
Copy link
Contributor

nenad-vujicic commented May 17, 2025

I'm not 100% sure merging of this PR is causing this, but take a look at how e.g. my profile page looks now (Edge, Chrome, .. every browser and user profile I tried):

image

@git-tgo
Copy link

git-tgo commented May 17, 2025

nenad-vujicic > ... looks now (Edge, Chrome ...

I can partly agree.

  logged in logged out
FF ok ok
Edge not ok ok
Chrome not ok ok

@tomhughes
Copy link
Member

Looks OK to me in both Firefox and Chromium on linux whether or not I am logged in.

@hlfan
Copy link
Collaborator Author

hlfan commented May 17, 2025

I think I've found the culprit:

if (date.getUTCDay() === weekInfo.firstDay) {

getUTCDay is [0,6], weekInfo.firstDay is [1,7], so the first shown week never starts for 0 === 7.

See #6018

@verhovsky
Copy link

verhovsky commented May 17, 2025

just 5 color buckets is easier to read than a continuous gradient

for example I can't see any of my contributions because in the last few months I've had days with a lot more contributions

Screenshot 2025-05-17 at 11 14 13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hide future days in user heatmap Add space and headline to user profile heatmap

7 participants